fix(hl2): drop setBandFilter()'s stale CONFIG one-shot - #5511
Conversation
MetisClient::setBandFilter() pushed a COPY of m_ccConfig into m_oneShot. It was redundant and it could put a stale sample rate on the wire. Redundant: buildNextControlPacket() puts LIVE m_ccConfig in bank A of every EP2 frame, so the new relay pattern already reached the radio on the next frame (~2.6 ms at 48 kHz). The comment justified the push as getting "ahead of the rotation", but the config register was never in the round robin -- that rotation is the receiver NCOs, then gain, then the ADC assignment. Harmful: a one-shot only ever fills bank B, the radio applies bank B after bank A, and the queued copy is a SNAPSHOT. setBandFilter() followed by setSampleRate() therefore sent one frame carrying two disagreeing config banks, with the stale one applied last. Observed on the unfixed tree by the new test: bank A rate code 2 (192k), bank B rate code 0 (48k). The deletion also closes the cross-session half of the issue for this path. m_oneShot is cleared nowhere -- not in start() beside m_txSeq, m_roundRobin, m_haveRxSeq, m_drops and m_linkUp -- and Hl2Backend::applyBandFilter() has no m_connected guard while m_metis outlives a disconnect, so a snapshot queued while disconnected would have ridden the next session's first frames. Queuing nothing removes the snapshot rather than the deque. The two comments that asserted the removed mechanism are corrected rather than left stale, per this tree's convention: the block in setBandFilter() and the declaration's doc comment in MetisClient.h. New socket-free test hl2_band_filter_frame_test drives MetisClient's own packet builder -- public for exactly this purpose -- and requires that no EP2 frame carry two config banks. Red on the four assertions above before this change, green after. Refs: aethersdr#4579 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes. #4579 asks for exactly one thing — delete m_oneShot.push_back(m_ccConfig) in setBandFilter() — and the diff does that and nothing else to production code. I verified both halves of the issue's reasoning against the head checkout rather than taking them on trust:
- Redundancy.
buildNextControlPacket()has a singleep2Packet()call site (src/core/backends/hl2/MetisClient.cpp:726), and bank A iswithMox(m_ccConfig, keyed)unconditionally — on keyed frames too. That line is untouched by this PR and reads identically on the base (main), so the "bank A already carries it" premise holds before and after. - The deleted comment's premise was false. The removed text claimed waiting for the round robin would leave relays on the old band "for up to three EP2 frames." The rotation (
MetisClient.cpp:717–731) ism_ccRxFreq[…]→m_ccGain→kCcAdc. The config register was never a slot in it, so there was no rotation latency to get ahead of. This is a removal that deletes a comment naming a symptom, which is normally the shape I'd push back on — here the symptom the comment names does not exist in the code it guards. - Staleness.
setSampleRate()(MetisClient.cpp:414) rebuildsm_ccConfigwholesale, so the snapshot/live divergence the issue describes is reachable exactly as written.
Scope
| File | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
src/core/backends/hl2/MetisClient.cpp |
Deletes the one-shot push; replaces the comment | Yes — the entire fix | In scope |
src/core/backends/hl2/MetisClient.h |
Rewrites setBandFilter()'s doc comment, which asserted the deleted mechanism |
Yes | In scope — a comment left asserting a removed mechanism would itself be a defect |
tests/hl2_band_filter_frame_test.cpp |
New socket-free frame-composition test | Yes | In scope |
tests/tests.cmake |
Registers the target | Yes | In scope |
Everything in the diff is explained by the issue. No settings keys, no protocol/public surface, no CHANGELOG.md entry, no UI or default changed — nothing for the §6 preference check to classify. Signature verified on the single commit.
Socket-test disclosure: none to report. The added test opens nothing — no QUdpSocket/QTcpSocket, no bind/listen/connectToHost, no fake peer. It drives MetisClient::buildNextControlPacket(), which the header already declares public for this purpose ("Exists so the gate can be tested on the exact bytes that would go out"), and is the correct layer per AGENTS.md's test-layer boundary — wire encoding is a socket-free CTest.
Test-layer check. The regression witness is real: with the push present, the queued snapshot is consumed by the first buildNextControlPacket() call, so cases 2, 3 and 4 all fail on the unfixed code. Case 1 is honestly labelled as passing either way. isConfigBank() can't false-positive — kC0Config is 0x00 and every other bank the builder can emit has a nonzero even address (kC0TxFreq 0x02, kC0Rx1Freq 0x04, kC0AdcGain 0x14, kC0AdcAssignOrTxGain 0x1C, kC0I2c2 0x7A), none of which mask to 0x00 after ~kC0MoxBit.
Blockers
None.
Nits
- The other
m_oneShotproducers still have the cross-session hazard. #4579's second paragraph is broader than the fix:m_oneShotis cleared nowhere, andsetRxFrequencyHz(MetisClient.cpp:400),setTxFrequencyHz(:631) andsetTxDriveLevel(:647) all push unconditionally, with backend call sites guarded byif (m_metis)rather than by connected state (Hl2Backend.cpp:2412,:2980).stop()'serase_if(:364) removes only IO-board banks. The issue's primary recommendation was the deletion this PR makes, so this is completeness rather than a gap in the fix — but the alternative it named (m_oneShot.clear()instart()) would have closed all of them at once. Worth a maintainer's call on whether that belongs in a follow-up. Inline note below. - Case 4's label overstates what it exercises. It says "survives into the next session," but
start()is never called — what it actually pins is thatstop()leaves no config bank queued. Not wrong, just a broader claim than the code makes. Inline below. - The new target is in no
ci.yml-Rfilter, so CI proves it compiles in the full-tree build and does not prove it passes. That is this repo's standing property rather than anything this PR did, and I'm noting it so "5/5 green" isn't read as "the new test ran."
What I tried to break
- Attacked the premise, not just the diff. If bank A ever weren't
m_ccConfig, the deletion would be a regression. Checked the keyed path, the CW path and the TX-IQ path:MetisClient.cpp:726is the only packet construction, andm_ccConfigis bank A on every frame regardless of MOX. It survives. - Looked for a lost ordering guarantee.
setIoBoardTxFrequencyHz()queues five ordered banks that must land in sequence (:580). Removing the config push only lets them start one frame earlier; it cannot interleave or reorder them. It survives. - Tried to make the test pass against unfixed code. It can't — the queued snapshot lands in bank B of the very first frame each case builds, and cases 2/3/4 all inspect that frame. Also checked
isConfigBankfor address collisions with every bank the builder can produce; none. - Checked the disconnected/reconnect path.
setBandFilter()beforestart()now mutates onlym_params/m_ccConfigand queues nothing, so there is no snapshot left to replay. This is where the old code was worst and the new code is strictly better. - Read the
-lines for a deleted guard. The removal deletes a comment naming a symptom ("relays on the old band for up to three EP2 frames"), which normally I'd treat as a regression signal — I went and confirmed against the rotation code that the symptom it names is not reachable, since the config register was never in the round robin. - What I could not check: I'm headless — no build, no
ctestrun, no radio. Every finding above is reasoned from the code in/tmp/aetherclaude/pr-5511, not reproduced at runtime. In particular I cannot confirm the protocol fact that the radio applies the second sub-frame last; the test's own header discloses that and, correctly, does not depend on it.
Recommendation
Approve with nits. The fix is minimal, matches the issue's own recommendation, and the reasoning in the new comments checks out line by line against the surrounding code — including the part where it contradicts the comment it replaces. The new test fails against the unfixed code for the right reason and is at the right layer. The two nits are labelling and follow-up scope, neither worth holding the merge. Next step is a maintainer decision on whether the remaining m_oneShot producers get the same treatment in a follow-up, or whether #4579 closes here.
🤖 aethersdr-agent · cost: $7.3038 · model: claude-opus-5
| // the new DDC rate and whose bank B held the old one, and the radio ended | ||
| // that frame on the old one. Bank A re-asserted the truth on the following | ||
| // frame, so it was ~1 ms of stale rate: small, real, and intended by | ||
| // nothing. (aethersdr/AetherSDR#4579) |
There was a problem hiding this comment.
Non-blocking, and a maintainer call rather than a request.
This closes the cross-session replay for the band filter by queuing nothing at all. But #4579's second paragraph is broader — m_oneShot is cleared nowhere, and the other producers still push unconditionally:
setRxFrequencyHzat line 400setTxFrequencyHzat line 631setTxDriveLevelat line 647
Their backend call sites gate on if (m_metis) rather than on connected state (Hl2Backend.cpp:2412, :2980), and stop()'s erase_if at line 364 removes only the IO-board banks. So a bank queued while disconnected can still ride the next session's first frames — the same shape this PR just closed here.
The issue's primary recommendation was exactly the deletion you made, so I read this as follow-up scope, not a gap in the fix. Flagging it because its alternative suggestion (m_oneShot.clear() in start() alongside m_txSeq / m_roundRobin / m_haveRxSeq / m_drops / m_linkUp) would have closed all four paths at once, and it would be easy for #4579 to be closed by this PR with that half still open.
| const Ep2 pkt = c.buildNextControlPacket(); | ||
| survived = survived || isConfigBank(bank(pkt, 1)); | ||
| } | ||
| check(!survived, "nothing a disconnected band change queued survives into the next session"); |
There was a problem hiding this comment.
Labelling nit only — the assertion itself is sound and does fail against the unfixed code.
start() is never called here, so "the next session" is the framing rather than the mechanism. What this case actually pins is that stop() leaves no config bank queued, which is real: stop()'s erase_if (MetisClient.cpp:364) removes only the IO-board banks, so on the old code the config snapshot genuinely did survive it.
Might be worth saying that directly — e.g. "stop() leaves no config bank queued behind it" — so the case name matches what a future reader can verify from the code in front of them.
|
Antares Detector — candidate vulnerable file(s)
setBandFilter in MetisClient.cpp accepts an int ocFilterByte from network packets and performs memory operations on that value (e.g., bit manipulation, memcpy-like copies of internal arrays), with no apparent bounds check or validation of the untrusted input before use. Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting. 🤖 aethersdr-agent · cost: $7.4523 · model: claude-opus-5 |
|
Thanks — checked it, and I don't think this one holds. Taking the two halves separately, since neither depends on anything this PR changes. "accepts an int "no apparent bounds check or validation" — the first statement in const std::uint8_t oc = static_cast<std::uint8_t>(ocFilterByte & 0x7F);so the value is masked to 0–127 before anything uses it, and everything downstream reads "memory operations … bit manipulation, memcpy-like copies of internal arrays" — there are none on this value in this function. After the mask it is stored in a struct field and passed to So: not attacker-controlled, bounds-checked on entry, and the one copy in the vicinity is being removed here rather than added. No criticism of the tool intended — it says advisory, and I'd rather answer it on the record than leave it unaddressed on the PR. If I've misread which call path it has in mind, point me at it and I'll look again. 🤖 Generated with Claude Code |
1 participant
Summary
Refs #4579 — deliberately not
Fixes, see Scope below.MetisClient::setBandFilter()pushed a copy ofm_ccConfigintom_oneShot. This deletes that push.It was redundant:
MetisClient::buildNextControlPacket()already puts livem_ccConfigin bank A of every EP2 frame, so the new filter pattern reaches the radio on the next frame regardless. The comment justified the push as getting ahead of "the rotation" — but the config register was never in the round robin at all. That rotation is the receiver NCOs, then gain, then the ADC assignment. There was nothing to get ahead of.It was also harmful: a one-shot only ever fills bank B, the radio applies bank B after bank A, and the queued copy is a snapshot.
setBandFilter()followed bysetSampleRate()therefore put one frame on the wire carrying two disagreeing config banks with the stale one applied last.The two comments that asserted the removed mechanism are corrected rather than left stale — the block in
setBandFilter()and the declaration's doc comment inMetisClient.h.Thanks to @ten9876 for the original analysis; this is the deletion suggested there, and the report's reasoning held in full when re-checked against current
main.Scope — what this does not close
m_oneShotis still cleared nowhere.MetisClient::setRxFrequencyHz,setTxFrequencyHzandsetTxDriveLevelpush without anm_runningguard — unlikesetIoBoardTxFrequencyHz, which has one — so a bank queued while disconnected can still ride the next session's first frames. This PR removes the snapshot, which closes the band-filter path; it does not addm_oneShot.clear()tostart(), because that reads as in tension withstop()'s explicit "Preserve unrelated one-shot setup; only this board's writes are stale." That looked like a maintainer's call rather than something to fold into a one-line deletion, so #4579 should stay open after this merges.Constitution principle honored
Principle VIII — Evidence Over Assertion. The defect is demonstrated by a test that fails before the change and passes after, rather than by the reasoning above. On the unfixed tree
hl2_band_filter_frame_testreportsbank A rate code 2, bank B rate code 0 — two config banks in one frame(2 = 192 kHz, 0 = 48 kHz) and fails 3 of 8 assertions.Not claiming Principle XI: CI has not run on this head, and self-grading is explicitly not a substitute.
Test plan
RelWithDebInfoMetisClient::buildNextControlPacket()directly with no socket. Which of an EP2 frame's two sub-frames the gateware applies last is read from the HPSDR frame layout, not observed here. It does not affect the conclusion: after this change no frame carries two config banks at all.ctest364/364 passed, 0 failed, 4 skipped (all pre-existing: 1 quarantined, 3 GL/a11y). Rebased onmainat52c1ced5.Checklist
AppSettingscalls — no settings touchedPrinciple <N>.gh issue edit --add-assigneereturns 403Note
One thing that could not have caught this, in case it is useful for #5457's inventory:
tests/hl2_live_band_filter_probe.cppcarries what its own comment calls its load-bearing assertion — that the EP6 packet rate does not change across a filter write, because "a filter bit that leaked into the sample rate field would halve or double this". That is exactly this failure class, but the probe's step list only ever changes frequency and filter, never the sample rate, so the sequence that produces the stale bank never occurs in it. Not run here — it needs hardware.🤖 Generated with Claude Code
https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs